refactor: remove -inner div element to simplify DOM structure #297
Conversation
Co-Authored-By: Claude (GLM-4.7) <noreply@anthropic.com>
Summary of ChangesHello @zombieJ, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the checkbox component by eliminating an unnecessary inner Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
演练此PR对复选框组件进行了结构和样式优化：简化了DOM层级结构，移除了内部装饰性跨度元素；同时重构了LESS样式表，将原嵌套的内部前缀类样式直接整合到主元素上，并优化了伪元素、悬停、禁用状态和动画的处理方式。 变更内容
估计审查工作量🎯 3 (中等复杂) | ⏱️ ~20 分钟 可能关联的PR
诗篇
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a nice refactoring that simplifies the DOM structure of the Checkbox component by removing an unnecessary inner <span>. The corresponding CSS changes are also well-executed, moving styles from the inner element to the main component wrapper. I've added a couple of suggestions to the stylesheet to further improve the code by using a CSS shorthand for brevity and to fix a minor pre-existing animation bug that became apparent during the refactor.
| border-color: #d9d9d9; | ||
| border-style: solid; | ||
| border-width: 1px; |
There was a problem hiding this comment.
For conciseness and better readability, consider using the border shorthand property to set the width, style, and color of the border in one declaration.
border: 1px solid #d9d9d9;
| .@{checkboxPrefixCls}-checked { | ||
| background-color: #3dbcf6; | ||
| border-color: #3dbcf6; | ||
|
|
||
| &:hover { | ||
| .@{checkboxInnerPrefixCls} { | ||
| border-color: #3dbcf6; | ||
| } | ||
| } | ||
|
|
||
| .@{checkboxInnerPrefixCls} { | ||
| border-color: #3dbcf6; | ||
| background-color: #3dbcf6; | ||
|
|
||
| &:after { | ||
| transform: rotate(45deg); | ||
| position: absolute; | ||
| left: 4px; | ||
| top: 1px; | ||
| display: table; | ||
| width: 5px; | ||
| height: 8px; | ||
| border: 2px solid #ffffff; | ||
| border-top: 0; | ||
| border-left: 0; | ||
| content: ' '; | ||
| animation-timing-function: cubic-bezier(0.68, -0.55, 0.27, 1.55); | ||
| animation-duration: 0.3s; | ||
| animation-name: amCheckboxOut; | ||
| } | ||
| } | ||
|
|
||
| @media print { | ||
| box-shadow: inset 0 0 0 16px #3dbcf6; | ||
| } | ||
| @media print { | ||
| box-shadow: inset 0 0 0 16px #3dbcf6; | ||
| } | ||
| } |
There was a problem hiding this comment.
This refactoring correctly preserves the existing styles. However, I noticed a potential pre-existing issue with the checkmark animation that could be fixed here. The checkmark currently fades out after appearing because of animation-name: amCheckboxOut on the base :after element. To fix this, you can override the animation for the checked state to use amCheckboxIn and ensure the final state persists. This will make use of the currently unused amCheckboxIn keyframes and provide a better user experience.
.@{checkboxPrefixCls}-checked {
background-color: #3dbcf6;
border-color: #3dbcf6;
&:after {
animation-name: amCheckboxIn;
animation-fill-mode: forwards;
}
&:hover {
border-color: #3dbcf6;
}
@media print {
box-shadow: inset 0 0 0 16px #3dbcf6;
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #297 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 30 30
Branches 5 5
=========================================
Hits 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@assets/index.less`:
- Around line 58-69: The checked state block for @{checkboxPrefixCls}-checked is
missing the &:after rule to apply the amCheckboxIn animation (the default :after
uses amCheckboxOut), so add an &:after nested rule inside
@{checkboxPrefixCls}-checked that sets animation-name: amCheckboxIn (and
matching animation-duration/animation-timing if used elsewhere) so the check
mark plays the "in" animation when checked; update the &:after selector that
references the pseudo-element and ensure it mirrors the properties used by the
default :after except with animation-name changed to amCheckboxIn.
- Around line 22-25: The current selector `&-input:focus + &` assumes the input
is a sibling of `.rc-checkbox` but the DOM in `src/index.tsx` places the input
inside `.rc-checkbox`, so the focus border style never matches; update the LESS
to target the parent when its child input is focused (e.g. replace the sibling
selector reference of `&-input:focus + &` with a parent selector that matches
`.rc-checkbox` containing the focused input, such as using
`:has(.rc-checkbox-input:focus)` on `.rc-checkbox`, or alternatively move the
focus style to the input and cascade it to the parent via a class toggle),
ensuring you reference the existing class names `&-input` / `.rc-checkbox` used
in the component.
🧹 Nitpick comments (1)
assets/index.less (1)
102-122: 动画关键帧定义正确，但存在厂商前缀不一致的小问题。第 37 行使用了
-webkit-transform前缀，但动画关键帧中只使用了标准transform。对于现代浏览器这不是问题，但如果需要支持较旧的 WebKit 浏览器，建议保持一致性。
| &:hover, | ||
| &-input:focus + & { | ||
| border-color: #3dbcf6; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.tsx" -o -name "*.jsx" -o -name "*.ts" -o -name "*.js" | grep -E "(index|checkbox)" | head -20Repository: react-component/checkbox
Length of output: 106
🏁 Script executed:
# Look for the checkbox component structure
rg -n "className.*rc-checkbox" --type tsx --type jsx -A 5 -B 5Repository: react-component/checkbox
Length of output: 95
🏁 Script executed:
# Search for input element and checkbox wrapper relationship
rg -n "rc-checkbox-input" --type tsx --type jsx -A 3 -B 3Repository: react-component/checkbox
Length of output: 95
🏁 Script executed:
cat -n ./src/index.tsx | head -100Repository: react-component/checkbox
Length of output: 3239
🏁 Script executed:
# Search for the input and checkbox element relationship
rg -n "rc-checkbox-input\|rc-checkbox" -A 5 -B 5Repository: react-component/checkbox
Length of output: 50
🏁 Script executed:
cat -n ./src/index.tsx | tail -20Repository: react-component/checkbox
Length of output: 611
🏁 Script executed:
# Check if there's more content after line 100
wc -l ./src/index.tsxRepository: react-component/checkbox
Length of output: 87
🏁 Script executed:
sed -n '90,110p' ./src/index.tsxRepository: react-component/checkbox
Length of output: 423
🏁 Script executed:
sed -n '1,50p' ./assets/index.lessRepository: react-component/checkbox
Length of output: 1152
修复 focus 选择器与 DOM 结构的不兼容问题。
选择器 &-input:focus + & 编译后为 .rc-checkbox-input:focus + .rc-checkbox，这是一个相邻兄弟选择器。然而，根据 src/index.tsx 的 DOM 结构，input 元素是 .rc-checkbox 的子元素而非兄弟元素，因此该选择器永远无法匹配。这导致 input 获得焦点时的边框颜色变化样式不会生效。
需要将选择器改为适配当前 DOM 结构的方案，例如使用 :has() 伪类或重构 DOM 关系。
🤖 Prompt for AI Agents
In `@assets/index.less` around lines 22 - 25, The current selector `&-input:focus
+ &` assumes the input is a sibling of `.rc-checkbox` but the DOM in
`src/index.tsx` places the input inside `.rc-checkbox`, so the focus border
style never matches; update the LESS to target the parent when its child input
is focused (e.g. replace the sibling selector reference of `&-input:focus + &`
with a parent selector that matches `.rc-checkbox` containing the focused input,
such as using `:has(.rc-checkbox-input:focus)` on `.rc-checkbox`, or
alternatively move the focus style to the input and cascade it to the parent via
a class toggle), ensuring you reference the existing class names `&-input` /
`.rc-checkbox` used in the component.
| .@{checkboxPrefixCls}-checked { | ||
| background-color: #3dbcf6; | ||
| border-color: #3dbcf6; | ||
|
|
||
| &:hover { | ||
| .@{checkboxInnerPrefixCls} { | ||
| border-color: #3dbcf6; | ||
| } | ||
| } | ||
|
|
||
| .@{checkboxInnerPrefixCls} { | ||
| border-color: #3dbcf6; | ||
| background-color: #3dbcf6; | ||
|
|
||
| &:after { | ||
| transform: rotate(45deg); | ||
| position: absolute; | ||
| left: 4px; | ||
| top: 1px; | ||
| display: table; | ||
| width: 5px; | ||
| height: 8px; | ||
| border: 2px solid #ffffff; | ||
| border-top: 0; | ||
| border-left: 0; | ||
| content: ' '; | ||
| animation-timing-function: cubic-bezier(0.68, -0.55, 0.27, 1.55); | ||
| animation-duration: 0.3s; | ||
| animation-name: amCheckboxOut; | ||
| } | ||
| } | ||
|
|
||
| @media print { | ||
| box-shadow: inset 0 0 0 16px #3dbcf6; | ||
| } | ||
| @media print { | ||
| box-shadow: inset 0 0 0 16px #3dbcf6; | ||
| } | ||
| } |
There was a problem hiding this comment.
选中状态缺少 amCheckboxIn 动画。
默认的 :after 伪元素设置了 animation-name: amCheckboxOut，但选中状态没有添加 &:after 规则来应用 amCheckboxIn 动画。这会导致勾选标记在选中时无法正确显示动画效果。
🐛 建议添加选中状态的动画规则
.@{checkboxPrefixCls}-checked {
background-color: `#3dbcf6`;
border-color: `#3dbcf6`;
+ &:after {
+ animation-name: amCheckboxIn;
+ }
+
&:hover {
border-color: `#3dbcf6`;
}
`@media` print {
box-shadow: inset 0 0 0 16px `#3dbcf6`;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .@{checkboxPrefixCls}-checked { | |
| background-color: #3dbcf6; | |
| border-color: #3dbcf6; | |
| &:hover { | |
| .@{checkboxInnerPrefixCls} { | |
| border-color: #3dbcf6; | |
| } | |
| } | |
| .@{checkboxInnerPrefixCls} { | |
| border-color: #3dbcf6; | |
| background-color: #3dbcf6; | |
| &:after { | |
| transform: rotate(45deg); | |
| position: absolute; | |
| left: 4px; | |
| top: 1px; | |
| display: table; | |
| width: 5px; | |
| height: 8px; | |
| border: 2px solid #ffffff; | |
| border-top: 0; | |
| border-left: 0; | |
| content: ' '; | |
| animation-timing-function: cubic-bezier(0.68, -0.55, 0.27, 1.55); | |
| animation-duration: 0.3s; | |
| animation-name: amCheckboxOut; | |
| } | |
| } | |
| @media print { | |
| box-shadow: inset 0 0 0 16px #3dbcf6; | |
| } | |
| @media print { | |
| box-shadow: inset 0 0 0 16px #3dbcf6; | |
| } | |
| } | |
| .@{checkboxPrefixCls}-checked { | |
| background-color: `#3dbcf6`; | |
| border-color: `#3dbcf6`; | |
| &:after { | |
| animation-name: amCheckboxIn; | |
| } | |
| &:hover { | |
| border-color: `#3dbcf6`; | |
| } | |
| `@media` print { | |
| box-shadow: inset 0 0 0 16px `#3dbcf6`; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@assets/index.less` around lines 58 - 69, The checked state block for
@{checkboxPrefixCls}-checked is missing the &:after rule to apply the
amCheckboxIn animation (the default :after uses amCheckboxOut), so add an
&:after nested rule inside @{checkboxPrefixCls}-checked that sets
animation-name: amCheckboxIn (and matching animation-duration/animation-timing
if used elsewhere) so the check mark plays the "in" animation when checked;
update the &:after selector that references the pseudo-element and ensure it
mirrors the properties used by the default :after except with animation-name
changed to amCheckboxIn.
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.